fix(api): apply or remove ignored tool parameters - #285
Conversation
|
A maintainer note on this whole series (#264–#285, #294), since this draft is its integration branch. First: thank you. The individual work here is genuinely good — where I deep-reviewed (#266, #278, #294), the root-cause analysis was accurate, the tests assert on real argv/fixtures rather than circularly on the code's own behavior, and #294 anticipates the exact safety hazard a reviewer would raise. Four PRs from this series are merged as of tonight (#266, #271, #274, #276), and more will follow. But the series was submitted as ~22 independent PRs and it is not internally coordinated, which creates real review hazards I need you to fix before the rest can land:
On identity: the branch prefixes say Codex, and the volume says automation. That's fine here — the code quality carries it — but the coordination failures above are exactly what an automated pipeline doesn't catch on its own. Run the cross-check between your own open PRs before filing, and this series becomes easy to take. |
Forward schematic and PCB monochrome settings, schematic theme and page selection, and 3D unspecified-model policy to kicad-cli. Restrict the BOM format contract to KiCad's CSV output. Refs mixelpixx#251
Narrow eight schemas to behavior their handlers can verify, correct overstated tool descriptions, and document a replacement workflow for every public input removal. Refs mixelpixx#251
Resolve project paths to their sibling PCB, query KiCad's real open-document list, and return an honest per-path result instead of ignoring open_project.path. Refs mixelpixx#251
Apply corner_radius as four circular KiCad arcs plus shortened sides over both IPC and the file fallback, rejecting overlapping or degenerate geometry. Refs mixelpixx#251
Validate supported standard pad shapes, update the pad header atomically, add or remove KiCad shape-specific children, and preserve an omitted size dimension. Refs mixelpixx#251
Validate timeout_seconds and apply one Tokio deadline to process detection plus IPC ping, reporting timeout state and elapsed time explicitly. Refs mixelpixx#251
Treat pcbnew and eeschema as GUI processes on every platform, and let a responsive IPC endpoint prove that KiCad is running even when process inspection misses its launcher. Report process detection separately for diagnostics.\n\nRefs mixelpixx#251
Make every IPC-backed routing operation resolve the advertised board path and refuse to operate unless that exact board is active in KiCad. This prevents a valid request from silently mutating whichever board happens to be open.\n\nRefs mixelpixx#251
Derive registered handlers and their transitive helper/macro call graph from source, then compare every advertised schema property with the implementation's parameter reads. Same-file resolution prevents duplicate helper names from hiding real drift, and the full catalogue check needs no allowlist.\n\nRefs mixelpixx#251
The component validator reported pins declared no-connect, ignored its public power-pin option, and projected every unit's pins through each placed unit. Preserve KiCad electrical types, honor the filter, exempt explicit and intrinsic no-connect pins, and resolve only the selected unit. Refs mixelpixx#182
ee6d47f to
262eb7d
Compare
nordic-style
changed the title
…mixelpixx#267 commit) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The #285 schema-parameter guard caught the refusing stub advertising four parameters its handler no longer reads — exactly the class it exists to catch, on its first day on main. Documented in docs/API_MIGRATIONS.md beside the #285 removals. Refs #315 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The community release: eighteen of the changes came from outside contributors. Repairs boards damaged by v0.4.0-v0.6.1 (#294), fixes set_board_size's outline stacking present since the first release (#314), makes move_connected honest (#315), enforces schema-parameter truth catalogue-wide (#285/#251), lands the shared connectivity index (#323), IPC-first board info with User paper dimensions (#207/#324), live zone creation (#316), the anyn99 schematic-fix series (#307-#313), sheet-pin rotation (#317), and neusse's developer docs (#306). ROADMAP rebuilt on discussion #165. 19 toolsets, 204 registered tools / 210 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
move_connected (mixelpixx#315, BoatDash backlog mixelpixx#17) refused unconditionally: since the first release it had silently delegated to move_schematic_component while reporting connections preserved. This implements the documented contract instead of merely re-wording the refusal, so `feat` rather than `fix`. Root cause: no code ever carried anything anchored at a moved symbol's old pin positions, and mixelpixx#315's own fix (183b43a) papered over that by refusing the call rather than building the carry. Change (crates/konnect-core/src/tools/sch_components.rs, crates/konnect-core/src/mcp/error.rs): - handle_move_connected now moves every placed unit (like move_schematic_component) and carries labels (net/global/hierarchical), power symbols, and no-connect flags whose position coincided with an old pin, plus the touching end of any wire. - A wire is stretched only if it stays horizontal or vertical; if any wire would go diagonal, the whole move is refused before anything is written, naming the wire(s) and a delta along their own axis that would stay orthogonal (new ToolErrorKind::WouldGoDiagonal). - Junction dots are re-judged by the same reconcile_junctions_after_move pass move_schematic_component already uses (mixelpixx#120) — no second opinion about junctions. - Response fields (moved_units, labels_moved_count, power_symbols_moved, no_connects_moved_count, wire_endpoints_moved_count, junctions_added/pruned_count) are all read from what was actually mutated, never echoed from the request. Tests (crates/konnect-core/src/tools/sch_components.rs, move_connected_tests): a four-pin fixture with a label/power symbol/no-connect/wire stub on each pin proves the carry and the orthogonal wire stretch, with response counts checked against a read-back of the written file; a second case proves the whole move is refused with nothing written when a wire would go diagonal (refusal path exercised before the success path); a third proves a multi-unit symbol moves every placed unit by the shared delta. Gates (see C:\GitRepos\Konnect-evidence\17-move-carries-attachments\README.md for the full before/after run against the io-expander corpus): netlist_diff identical partition after a move that used to break it, sch_layout_check 0 findings, ERC unchanged; diagonal case refused with the file byte-identical to before the call. CI-equivalent: cargo fmt --check clean; cargo clippy -D warnings clean; cargo test --workspace --lib --tests 1001+ passed, 0 failed; cargo test --workspace --doc all passed. Neuter check: disabling the diagonal-refusal guard alone fails a_move_that_would_put_a_wire_off_axis_is_refused_and_nothing_is_written; disabling the carry logic alone (via a shadowed at_old_pin closure) fails labels_power_and_no_connects_are_carried_and_the_orthogonal_wire_end_is_stretched. Both were restored to the exact original text afterward. docs/API_MIGRATIONS.md: updated the mixelpixx#285 removal entry to note the parameters are live again now that the feature is implemented. Part of mixelpixx#315 (mixelpixx/Konnect): the maintainer's comment on that issue says a wire-carrying move needs the mixelpixx#120 connectivity model, which reconcile_junctions_after_move already provides — the acceptance criteria in issue-draft.md track the remainder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
User-visible problem and scope
Konnect advertised public MCP inputs that some handlers never read. Clients could therefore receive a success response even though the requested behavior was not applied.
This PR rebases and consolidates the remaining #251 fixes on current
main. Each advertised input is now either implemented or deliberately removed with migration guidance. A catalogue-wide regression test prevents the same bug class from returning.Closes #251
Consolidated changes
The branch also includes #267's smaller
ignore_power_pinsimplementation as a temporary stacked prerequisite: the guard correctly detects that existing ignored input. #280 is intentionally excluded as its duplicate. Once #267 lands, this branch can be rebased to remove the duplicate commit.Already-merged #266, #271, #274, and #276 are inherited from
mainand are not duplicated here. #272 and #280 are not part of this branch.Root cause and design
The public schemas and their handlers previously evolved independently, with no catalogue-wide proof that every input reached executable code. The new test starts from the registered tool catalogue, locates each real handler, and follows reachable helpers and macros. Its lexer ignores comments and ordinary, raw, byte, and character literals so examples cannot masquerade as implementation. Same-file helper resolution wins when names are duplicated; cross-file helpers remain discoverable when no local definition exists.
Every schema property must appear in its handler's reachable implementation. There is no tool/parameter allowlist. The complete 202-tool catalogue is checked in about 0.24 seconds locally.
Compatibility and migration
Implemented inputs keep their public names and defaults and now affect the requested operation. Eight previously inert inputs are removed:
import_sheet_pins.project_name,refill_zones.zones,run_drc.tests,audit_decoupling.board,audit_decoupling.max_distance_mm,export_manufacturing_package.quantity,validate_for_manufacturing.schematic, andestimate_cost.schematic.Each removal and its supported replacement workflow is documented in
docs/API_MIGRATIONS.mdand enforced byschema_migrations.rs.Validation
All commands required by
CONTRIBUTING.mdpass on this exact consolidated branch:cargo test --workspace --locked --lib --testscargo test --workspace --locked --doccargo clippy --workspace --locked --all-targets -- -D warningscargo fmt --all -- --checkThe focused catalogue guard also passes 5/5 with
cargo test -p konnect --test schema_parameter_usage --locked -- --nocapture. Live-GUI tests remain intentionally ignored by the upstream suite unless a running KiCad GUI/socket is supplied.Risk and rollback
This is intentionally an integration PR, but the runtime fixes remain separated into focused commits. The main risks are incorrect KiCad CLI argument mapping, geometry conversion, board identity checks, and UI-process detection. Direct argv tests, real-format fixtures, and focused regressions cover those paths. The guard's static call-graph risk is covered by lexer, helper, macro, and duplicate-name tests. Any runtime area can be rolled back independently by reverting its focused commit.